Remove `thiserror` from `gix-transport`, use `gix-error` #2360

Copilot · 2026-01-09T20:51:12Z

Address review feedback on error handling
- Add separate Curl and Reqwest error variants instead of converting to String
- Update IsSpuriousError to use original curl_is_spurious and reqwest-specific checks
- Remove unused gix-error dependency from Cargo.toml
- Preserve error source information for debugging
All tests pass
Clippy clean

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

- Remove thiserror dependency from gix-transport - Create crate-level Error enum with simple variants - Convert all error types to use crate::Error - Implement From traits for error conversions - Update all error usage throughout the crate - Tests pass successfully Co-authored-by: Byron <[email protected]>

Co-authored-by: Byron <[email protected]>

Copilot

Pull request overview

This pull request aims to replace thiserror with a custom error handling approach in gix-transport, creating a unified Error enum to enable better error introspection for retry-ability checks. However, the implementation has several critical issues that need to be addressed.

Changes:

Created a crate-level Error enum with variants covering all transport error cases
Converted all existing error types to type aliases pointing to crate::Error
Implemented IsSpuriousError trait on the new error type
Added gix-error as a dependency (though it's not actually used)
Removed thiserror dependency

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
gix-transport/src/lib.rs	Added error module export
gix-transport/src/error.rs	New unified Error enum with Display, std::error::Error, and IsSpuriousError implementations
gix-transport/src/client/non_io_types.rs	Converted error types to aliases of crate::Error
gix-transport/src/client/git/blocking_io.rs	Converted connect::Error to type alias
gix-transport/src/client/capabilities.rs	Updated to use crate::Error directly
gix-transport/src/client/blocking_io/ssh/tests.rs	Updated test to match renamed error variant (Unsupported → SshUnsupported)
gix-transport/src/client/blocking_io/ssh/program_kind.rs	Updated error construction to use renamed variant
gix-transport/src/client/blocking_io/ssh/mod.rs	Converted SSH errors to type alias
gix-transport/src/client/blocking_io/http/traits.rs	Converted HTTP error to type alias
gix-transport/src/client/blocking_io/http/reqwest/remote.rs	Updated error handling to use unified error type
gix-transport/src/client/blocking_io/http/redirect.rs	Converted redirect error to type alias
gix-transport/src/client/blocking_io/http/mod.rs	Updated error construction for HTTP detail errors
gix-transport/src/client/blocking_io/http/curl/remote.rs	Removed redundant From implementations
gix-transport/src/client/blocking_io/http/curl/mod.rs	Converted curl error to type alias
gix-transport/src/client/blocking_io/file.rs	Simplified error propagation
gix-transport/Cargo.toml	Added gix-error dependency, removed thiserror
Cargo.lock	Updated dependency lock file

Copilot · 2026-01-10T09:33:35Z

gix-transport/src/error.rs

+impl crate::IsSpuriousError for Error {
+    fn is_spurious(&self) -> bool {
+        match self {
+            Error::Io(err) => err.is_spurious(),
+            Error::PostBody(err) => err.is_spurious(),
+            Error::ReadPostBody(err) => err.is_spurious(),
+            Error::InvokeProgram { source, .. } => source.is_spurious(),
+            Error::Connection(_) => true, // Connection errors are generally retry-able
+            _ => false,
+        }
+    }
+}


The IsSpuriousError implementation marks all Error::Connection(_) errors as spurious (retry-able), but this is overly broad and changes existing behavior.

Previously, only specific curl errors (connection failures, timeouts, etc.) were marked as spurious via the curl_is_spurious function. With the new implementation, ALL errors that get converted to Error::Connection(_) are marked as spurious, including:

All curl errors (not just network-related ones)

All reqwest errors (previously only timeout/connect/server errors were spurious)

Any boxed error (via the From<Box<dyn std::error::Error + Send + Sync>> impl)

This could lead to retrying errors that should not be retried. The spurious detection logic needs to preserve the original granularity, or the error type needs separate variants for spurious vs non-spurious connection failures.

Copilot · 2026-01-10T09:33:35Z

gix-transport/src/error.rs

+#[cfg(feature = "http-client-curl")]
+impl From<curl::Error> for Error {
+    fn from(err: curl::Error) -> Self {
+        Error::Connection(err.to_string())
+    }
+}


Converting all curl errors to Error::Connection(String) loses important error information. The original curl error type provides methods like is_couldnt_connect(), is_operation_timedout(), etc. that are used elsewhere in the codebase (see curl_is_spurious function).

By converting to a String, we lose:

The ability to distinguish between different types of curl errors

The error chain/source information

The ability to use curl's error type methods for error classification

Consider keeping the curl::Error as a separate variant (e.g., Curl(curl::Error)) to preserve this information for downstream error handling and debugging.

Copilot · 2026-01-10T09:33:35Z

gix-transport/src/error.rs

+#[cfg(feature = "http-client-reqwest")]
+impl From<reqwest::Error> for Error {
+    fn from(err: reqwest::Error) -> Self {
+        Error::Connection(err.to_string())
+    }
+}


Converting all reqwest errors to Error::Connection(String) loses important error information. The reqwest::Error type provides methods like is_timeout(), is_connect(), status(), etc. that were previously used for error classification.

By converting to a String, we lose:

The ability to distinguish between different types of HTTP errors (timeouts, connection failures, server errors, client errors, etc.)

HTTP status code information

The error chain/source information

The ability to use reqwest's error type methods for error classification

Consider keeping the reqwest::Error as a separate variant (e.g., Reqwest(reqwest::Error)) to preserve this information for downstream error handling and debugging.

Copilot · 2026-01-10T09:33:36Z

gix-transport/src/error.rs

+            Error::Io(err) => write!(f, "IO error: {err}"),
+            Error::UrlParse(err) => write!(f, "URL parse error: {err}"),
+            Error::PathConversion(err) => write!(f, "The git repository path could not be converted to UTF8: {err}"),
+            Error::UnsupportedScheme(scheme) => write!(f, "The '{scheme}' protocol is currently unsupported"),


The error message for UnsupportedScheme changed from including the full URL to just the scheme.

Old message: "The scheme in "" is not usable for an ssh connection"
New message: "The '' protocol is currently unsupported"

This loses valuable context about which specific URL caused the error. The old SSH-specific error provided the complete URL, which is more helpful for debugging. Consider preserving the full URL in the error or adding it as an additional field.

Copilot · 2026-01-10T09:33:36Z

gix-transport/Cargo.toml

 gix-packetline = { version = "^0.20.0", path = "../gix-packetline" }
 gix-credentials = { version = "^0.34.1", path = "../gix-credentials", optional = true }
 gix-quote = { version = "^0.6.1", path = "../gix-quote" }
+gix-error = { version = "^0.0.0", path = "../gix-error" }


The gix-error dependency is added but never actually imported or used in the codebase. The code defines its own Error enum and uses the IsSpuriousError trait that's already defined in gix-transport/src/lib.rs, not from gix-error.

Either:

Remove the unused gix-error dependency, or

Actually use types/traits from gix-error if that was the intended design

Based on the PR description mentioning "use gix-error", it seems like the intent was to use it, but the implementation doesn't actually import anything from it.

Suggested change

gix-error = { version = "^0.0.0", path = "../gix-error" }

Byron · 2026-01-10T11:43:27Z

@copilot Address everything brought up in the review.

…dency - Add separate Curl and Reqwest error variants to preserve error information - Update is_spurious to use specific curl/reqwest error checking - Remove unused gix-error dependency - Preserve error source chains for better debugging Co-authored-by: Byron <[email protected]>

Copilot · 2026-01-10T11:49:40Z

@copilot Address everything brought up in the review.

Addressed all actionable review feedback in commit 51e5126:

IsSpuriousError granularity restored: Changed from marking all Connection(_) as spurious to using specific error variants:
- Added separate Curl(curl::Error) and Reqwest(reqwest::Error) variants
- Updated is_spurious() to call curl_is_spurious() for curl errors
- Updated is_spurious() to check is_timeout(), is_connect(), and server errors for reqwest
Error information preserved: Kept curl::Error and reqwest::Error as typed variants instead of converting to strings, preserving:
- Error methods like is_couldnt_connect(), is_timeout(), etc.
- Error source chains for debugging
- HTTP status code information
Removed unused dependency: Removed gix-error from Cargo.toml as it wasn't actually being used
Error source chains: Updated std::error::Error::source() implementation to return sources for Curl and Reqwest variants

All tests pass and clippy is clean.

Byron · 2026-01-11T19:05:20Z

Let's just do it again, too much changed.

Initial plan

2cb6537

Copilot AI assigned Copilot and Byron Jan 9, 2026

Copilot AI mentioned this pull request Jan 9, 2026

gix-error punch-through #2352

Merged

6 tasks

Copilot started work on behalf of Byron January 9, 2026 20:51 View session

Copilot AI and others added 3 commits January 9, 2026 21:06

Fix clippy warning in gix-transport

12f251d

Co-authored-by: Byron <[email protected]>

Remove redundant to_owned() call

10a2d6a

Co-authored-by: Byron <[email protected]>

Copilot AI changed the title ~~[WIP] Fix gix-error punch-through issue~~ Remove thiserror from gix-transport, use gix-error Jan 9, 2026

Copilot AI requested a review from Byron January 9, 2026 21:15

Copilot finished work on behalf of Byron January 9, 2026 21:15

Byron requested a review from Copilot January 10, 2026 09:24

Copilot started reviewing on behalf of Byron January 10, 2026 09:24 View session

Copilot AI reviewed Jan 10, 2026

View reviewed changes

Copilot started work on behalf of Byron January 10, 2026 11:44 View session

Copilot finished work on behalf of Byron January 10, 2026 11:50

Byron closed this Jan 11, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Remove `thiserror` from `gix-transport`, use `gix-error` #2360

Remove `thiserror` from `gix-transport`, use `gix-error` #2360

Uh oh!

Copilot AI commented Jan 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 10, 2026

Uh oh!

Copilot AI Jan 10, 2026

Uh oh!

Copilot AI Jan 10, 2026

Uh oh!

Copilot AI Jan 10, 2026

Uh oh!

Copilot AI Jan 10, 2026

Uh oh!

Byron commented Jan 10, 2026

Uh oh!

Copilot AI commented Jan 10, 2026

Uh oh!

Byron commented Jan 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Remove thiserror from gix-transport, use gix-error #2360

Remove thiserror from gix-transport, use gix-error #2360

Uh oh!

Conversation

Copilot AI commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

Uh oh!

Byron commented Jan 10, 2026

Uh oh!

Copilot AI commented Jan 10, 2026

Uh oh!

Byron commented Jan 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Remove `thiserror` from `gix-transport`, use `gix-error` #2360

Remove `thiserror` from `gix-transport`, use `gix-error` #2360

Copilot AI commented Jan 9, 2026 •

edited

Loading